Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle fallible per commitment point in channel establishment #3109

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

alecchendev
Copy link
Contributor

@alecchendev alecchendev commented Jun 6, 2024

Builds on #3086, #3096, #3115, #3149, #3150. First PR to handle fallible get_per_commitment_point signer method.

We make get_per_commitment_point return a result type so that in the Err case, we (LDK) can resume operation while an async signer fetches the commitment point. This will typically block sending out a message, but otherwise most state updates will occur. When the signature arrives, the user is expected to call ChannelManager::signer_unblocked and we will retry get_per_commitment_point however this time the signer will return the commitment point with Ok, and we'll send out our message.

This PR implements this flow for channel establishment, i.e. open_channel, accept_channel, and channel_ready.

Rough outline of how our HolderCommitmentPoint advances and gets new signatures:

  • sending open_channel - creates new outbound channel, immediately requests the first commit point, waits to have the first 2 commit points to be unblocked to send the message
  • sending accept_channel - same ^
  • sending funding_created - no op regarding commit points
  • receiving funding_created - uses point to verify first commitment, then advances commitment, requesting next point
  • sending funding_signed - no op
  • receiving funding_signed - same as above, verifies, advances, requests next point
  • sending channel_ready - waits to have the next commit point ready, then once unblocked sends the current point in the channel_ready message

To do

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 70.88123% with 76 lines in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (3ccf064) to head (744f2ca).
Report is 2 commits behind head on main.

Current head 744f2ca differs from pull request most recent head 0bdd4cc

Please upload reports for the commit 0bdd4cc to get more accurate results.

Files Patch % Lines
lightning/src/util/test_channel_signer.rs 36.53% 33 Missing ⚠️
lightning/src/ln/channel.rs 80.37% 29 Missing and 2 partials ⚠️
lightning/src/ln/functional_test_utils.rs 0.00% 6 Missing ⚠️
lightning/src/ln/channelmanager.rs 87.87% 0 Missing and 4 partials ⚠️
lightning/src/util/test_utils.rs 50.00% 0 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3109      +/-   ##
==========================================
- Coverage   89.81%   89.78%   -0.03%     
==========================================
  Files         121      119       -2     
  Lines       99314    97966    -1348     
  Branches    99314    97966    -1348     
==========================================
- Hits        89195    87955    -1240     
+ Misses       7514     7435      -79     
+ Partials     2605     2576      -29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alecchendev alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 1e5b210 to 07991fa Compare June 10, 2024 01:16
@alecchendev
Copy link
Contributor Author

alecchendev commented Jun 11, 2024

the commits from #3115 are second from the end, will rebase on top later this week, but generally most of the stuff here is in place

@alecchendev alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 07991fa to 744f2ca Compare June 11, 2024 01:22
@alecchendev alecchendev force-pushed the 2024-06-async-commit-point-funding branch 3 times, most recently from 73583e7 to b798ffa Compare June 18, 2024 22:24
@alecchendev alecchendev marked this pull request as ready for review June 18, 2024 22:27
// TODO: add a variant for before our first commitment point is retrieved
/// We have just created or accepted a channel and are pending our very
/// first commitment point.
Uninitialized { transaction_number: u64 },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to first handle all the async cases post-funding and then add support for handling the async case during initial channel bring up in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay, yea you're right, i just forgot! will be back to prioritizing this next week

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just put up #3150 - will rebase this PR on top of that when i get time

This attempts to refine the signer pending flag mutations +
move some signer flag logic into the `signer_unblocked`
method instead of our other lightning logic.

Before flow:
- Call function that asks for signature
	- if absent, sets signer pending flag
	- if present, clear signer pending flag
- Call `signer_unblocked` -> call the function again, and it'll set the
  flags again depending on the result

After flow:
- Call function that asks for signature
	- if absent, sets signer pending flag
- Call `signer_unblocked` -> clear the signer pending flag ->
  call the function again, and it only resets the flag if unsuccessful

This moves the logic for all async commitment signatures, i.e.
funding_created, funding_signed, and commitment_signed.
Includes simple changes to test util signers and tests, as well as
handling the error case for get_per_commitment_point in
HolderCommitmentPoint. This leaves a couple `.expect`s in places
that will be addressed in a separate PR for handling funding.
Note: this does not test the CS -> RAA resend ordering, because this
requires handling async get_per_commitment_point for channel
reestablishment, which will be addressed in a follow up PR.
@alecchendev alecchendev force-pushed the 2024-06-async-commit-point-funding branch from b798ffa to 0bdd4cc Compare July 2, 2024 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants